schedulers,test: avoid some test branches not being reached and remove schedulePeerPr #8087

lhy1024 · 2024-04-17T14:02:17Z

What problem does this PR solve?

Issue Number: Close #8073

What is changed and how does it work?

schedulePeerPr always be 1.0, which makes some test cannot reach transfer-leader branch

Check List

Tests

Unit test

Release note

None.

ti-chi-bot · 2024-04-17T14:02:20Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

HuSharp

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

lhy1024 · 2024-04-17T14:04:25Z

pkg/schedule/schedulers/hot_region_test.go

@@ -527,25 +534,12 @@ func checkHotWriteRegionScheduleByteRateOnly(re *require.Assertions, enablePlace
 	clearPendingInfluence(hb.(*hotScheduler))
 	tc.SetHotRegionScheduleLimit(int(opt.GetHotRegionScheduleLimit()))

-	for i := 0; i < 20; i++ {


This part is duplicated with L500-L522. But this branch, which tests the transfer leader, is not reached, so it is not updated before.

lhy1024 · 2024-04-17T14:04:58Z

pkg/schedule/schedulers/hot_region_test.go

+	pdServerCfg := tc.GetPDServerConfig()
+	pdServerCfg.FlowRoundByDigit = 8


Add config to avoid test branches not being reached

lhy1024 · 2024-04-17T14:14:04Z

pkg/schedule/schedulers/hot_region_test.go

-			re.Equal("move-hot-write-leader", op.Desc())
-			operatorutil.CheckTransferLearner(re, op, operator.OpHotRegion, 8, 10)
+			re.Equal("move-hot-write-peer", op.Desc())
+			operatorutil.CheckTransferLearner(re, op, operator.OpHotRegion, 8, 0)


it may be scheduled to store 10 or store 11

Is it possible to make the result deterministic？

codecov · 2024-04-17T15:06:20Z

Codecov Report

Attention: Patch coverage is 82.69231% with 9 lines in your changes missing coverage. Please review.

Project coverage is 77.12%. Comparing base (6b25787) to head (8d6f0cc).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8087      +/-   ##
==========================================
- Coverage   77.21%   77.12%   -0.10%     
==========================================
  Files         470      470              
  Lines       61671    61681      +10     
==========================================
- Hits        47622    47574      -48     
- Misses      10468    10516      +48     
- Partials     3581     3591      +10

Flag	Coverage Δ
unittests	`77.12% <82.69%> (-0.10%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

rleungx · 2024-05-28T03:19:47Z

pkg/schedule/schedulers/hot_region.go

 	// schedulePeerPr the probability of schedule the hot peer.
-	schedulePeerPr = 0.66
+	schedulePeerPr = defaultSchedulePeerPr
 	// pendingAmpFactor will amplify the impact of pending influence, making scheduling slower or even serial when two stores are close together
-	pendingAmpFactor = 2.0
+	pendingAmpFactor = defaultPendingAmpFactor
 	// If the distribution of a dimension is below the corresponding stddev threshold, then scheduling will no longer be based on this dimension,
 	// as it implies that this dimension is sufficiently uniform.
-	stddevThreshold = 0.1
+	stddevThreshold = defaultStddevThreshold
 	// topnPosition is the position of the topn peer in the hot peer list.
 	// We use it to judge whether to schedule the hot peer in some cases.
-	topnPosition = 10
+	topnPosition = defaultTopnPosition


These variables are too complicated. Moreover, the current hot tests are hard to understand and debug.

Yes, I will try to reduce these variables and to make test clear in later prs.

Signed-off-by: lhy1024 <admin@liudos.us>

lhy1024 · 2024-06-27T07:59:17Z

@rleungx @HuSharp PTAL

Signed-off-by: lhy1024 <admin@liudos.us>

rleungx · 2024-06-27T06:50:32Z

pkg/schedule/schedulers/scheduler_test.go

-	ops, _ = hs.Schedule(tc, false)
-	re.Len(ops, 1)
-	operatorutil.CheckTransferPeer(re, ops[0], operator.OpHotRegion, 1, 4)
+	for i := 0; i < 100; i++ {


Why change it?

pd/pkg/schedule/schedulers/hot_region.go

Lines 409 to 430 in e7c9d15

func (h *hotScheduler) balanceHotWriteRegions(cluster sche.SchedulerCluster) []*operator.Operator {

// prefer to balance by peer

s := h.r.Intn(100)

switch {

case s < int(schedulePeerPr*100):

peerSolver := newBalanceSolver(h, cluster, utils.Write, movePeer)

ops := peerSolver.solve()

if len(ops) > 0 && peerSolver.tryAddPendingInfluence() {

return ops

}

default:

}

leaderSolver := newBalanceSolver(h, cluster, utils.Write, transferLeader)

ops := leaderSolver.solve()

if len(ops) > 0 && leaderSolver.tryAddPendingInfluence() {

return ops

}

hotSchedulerSkipCounter.Inc()

return nil

}

In master code, if a write peer was randomized but no create operator was created, another attempt was made to generate a write leader.
Now, to simplify the code, there is no such logic.

So I added this check to avoid meeting panic when randomized write peer.

rleungx · 2024-07-01T05:56:53Z

pkg/schedule/schedulers/hot_region.go

@@ -121,7 +122,7 @@ type baseHotScheduler struct {
 	// this records regionID which have pending Operator by operation type. During filterHotPeers, the hot peers won't
 	// be selected if its owner region is tracked in this attribute.
 	regionPendings  map[uint64]*pendingInfluence
-	types           []utils.RWType
+	types           []resourceType


How about using a more clear name instead of resourceType?

Do you have any good ideas about this name?

Maybe we can merge rwTy and resourceType in another PR?

Maybe we can merge rwTy and resourceType in another PR?

+1

rleungx · 2024-07-01T05:58:18Z

pkg/schedule/schedulers/hot_region.go

 		return h.balanceHotReadRegions(cluster)
-	case utils.Write:
-		return h.balanceHotWriteRegions(cluster)
+	case writePeer:


Why do we need to distinguish write?

pd/pkg/statistics/collector.go

Line 68 in e7c9d15

// while the Leader and Follower are under different loads (usually the Leader consumes more CPU).

So we schedule the write leader based on the query, and we schedule the write peer based on the byte and key at the moment.

rleungx · 2024-07-01T06:00:21Z

pkg/schedule/schedulers/hot_region_test.go

-			re.Equal("move-hot-write-leader", op.Desc())
-			operatorutil.CheckTransferLearner(re, op, operator.OpHotRegion, 8, 10)
+			re.Equal("move-hot-write-peer", op.Desc())
+			operatorutil.CheckTransferLearner(re, op, operator.OpHotRegion, 8, 0)


Is it possible to make the result deterministic？

Signed-off-by: lhy1024 <admin@liudos.us>

lhy1024 · 2024-07-03T03:24:30Z

/test pull-integration-realcluster-test

ti-chi-bot · 2024-07-04T02:45:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: HuSharp, rleungx

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [HuSharp,rleungx]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2024-07-04T02:45:48Z

[LGTM Timeline notifier]

Timeline:

2024-07-03 03:35:59.004964433 +0000 UTC m=+1381885.490453264: ☑️ agreed by rleungx.
2024-07-04 02:45:47.588184446 +0000 UTC m=+1465274.073673278: ☑️ agreed by HuSharp.

ti-chi-bot bot added the release-note-none Denotes a PR that doesn't merit a release note. label Apr 17, 2024

ti-chi-bot bot requested review from JmPotato and rleungx April 17, 2024 14:02

ti-chi-bot bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 17, 2024

lhy1024 commented Apr 17, 2024

View reviewed changes

HuSharp approved these changes May 20, 2024

View reviewed changes

ti-chi-bot bot added the status/LGT1 Indicates that a PR has LGTM 1. label May 20, 2024

rleungx reviewed May 28, 2024

View reviewed changes

ti-chi-bot bot added dco-signoff: yes Indicates the PR's author has signed the dco. approved labels Jun 25, 2024

lhy1024 added 3 commits June 25, 2024 19:13

tests: avoid some test branches not being reached

6f1103a

Signed-off-by: lhy1024 <admin@liudos.us>

make test stable

a91615b

Signed-off-by: lhy1024 <admin@liudos.us>

remove schedulePeerPr

ccbbef1

Signed-off-by: lhy1024 <admin@liudos.us>

lhy1024 force-pushed the fix-test20 branch from 9aa8c16 to ccbbef1 Compare June 25, 2024 12:45

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 25, 2024

lhy1024 changed the title ~~tests: avoid some test branches not being reached~~ schedulers,test: avoid some test branches not being reached and remove schedulePeerPr Jun 25, 2024

lhy1024 requested review from rleungx and HuSharp June 25, 2024 12:55

remove comments

73aab70

Signed-off-by: lhy1024 <admin@liudos.us>

lhy1024 added 2 commits June 27, 2024 16:19

Merge branch 'master' of github.com:tikv/pd into fix-test20

6e7cb84

add more tests

f4f4e04

Signed-off-by: lhy1024 <admin@liudos.us>

rleungx reviewed Jul 1, 2024

View reviewed changes

lhy1024 added 2 commits July 2, 2024 18:03

address comments

f7d8c25

Signed-off-by: lhy1024 <admin@liudos.us>

update

f434c5c

Signed-off-by: lhy1024 <admin@liudos.us>

rleungx approved these changes Jul 3, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jul 3, 2024

HuSharp approved these changes Jul 4, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jul 4, 2024

Merge branch 'master' into fix-test20

8d6f0cc

ti-chi-bot bot merged commit 86f8030 into tikv:master Jul 4, 2024
25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

schedulers,test: avoid some test branches not being reached and remove schedulePeerPr #8087

schedulers,test: avoid some test branches not being reached and remove schedulePeerPr #8087

lhy1024 commented Apr 17, 2024 •

edited

Loading

ti-chi-bot bot commented Apr 17, 2024 •

edited

Loading

lhy1024 Apr 17, 2024

lhy1024 Apr 17, 2024

lhy1024 Apr 17, 2024

rleungx Jul 1, 2024

lhy1024 Jul 2, 2024

codecov bot commented Apr 17, 2024 •

edited

Loading

rleungx May 28, 2024

lhy1024 May 28, 2024 •

edited

Loading

lhy1024 commented Jun 27, 2024

rleungx Jun 27, 2024

lhy1024 Jul 1, 2024 •

edited

Loading

lhy1024 Jul 1, 2024

rleungx Jul 1, 2024

lhy1024 Jul 1, 2024

lhy1024 Jul 2, 2024

HuSharp Jul 4, 2024

rleungx Jul 1, 2024

lhy1024 Jul 1, 2024

rleungx Jul 1, 2024

lhy1024 commented Jul 3, 2024

ti-chi-bot bot commented Jul 4, 2024

ti-chi-bot bot commented Jul 4, 2024

		pdServerCfg := tc.GetPDServerConfig()
		pdServerCfg.FlowRoundByDigit = 8

	func (h hotScheduler) balanceHotWriteRegions(cluster sche.SchedulerCluster) []operator.Operator {
	// prefer to balance by peer
	s := h.r.Intn(100)
	switch {
	case s < int(schedulePeerPr*100):
	peerSolver := newBalanceSolver(h, cluster, utils.Write, movePeer)
	ops := peerSolver.solve()
	if len(ops) > 0 && peerSolver.tryAddPendingInfluence() {
	return ops
	}
	default:
	}

	leaderSolver := newBalanceSolver(h, cluster, utils.Write, transferLeader)
	ops := leaderSolver.solve()
	if len(ops) > 0 && leaderSolver.tryAddPendingInfluence() {
	return ops
	}

	hotSchedulerSkipCounter.Inc()
	return nil
	}

schedulers,test: avoid some test branches not being reached and remove schedulePeerPr #8087

schedulers,test: avoid some test branches not being reached and remove schedulePeerPr #8087

Conversation

lhy1024 commented Apr 17, 2024 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 17, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

lhy1024 May 28, 2024 • edited Loading

Choose a reason for hiding this comment

lhy1024 commented Jun 27, 2024

Choose a reason for hiding this comment

lhy1024 Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhy1024 commented Jul 3, 2024

ti-chi-bot bot commented Jul 4, 2024

ti-chi-bot bot commented Jul 4, 2024

[LGTM Timeline notifier]

lhy1024 commented Apr 17, 2024 •

edited

Loading

ti-chi-bot bot commented Apr 17, 2024 •

edited

Loading

codecov bot commented Apr 17, 2024 •

edited

Loading

lhy1024 May 28, 2024 •

edited

Loading

lhy1024 Jul 1, 2024 •

edited

Loading